Skip to content
This repository has been archived by the owner on Nov 18, 2024. It is now read-only.

Add account sheet #37

Merged
merged 8 commits into from
Feb 1, 2023
Merged

Add account sheet #37

merged 8 commits into from
Feb 1, 2023

Conversation

elisealix22
Copy link
Contributor

This PR updates our "Account" view to a sheet and updates the designs to closer match Figma. I made the privacy URL link to https://xmtp.org/privacy and the support url link to https://github.com/xmtp-labs/xmtp-inbox-ios/issues.

I also migrated our fetch/stream messages logic into the MessageListView to match how we are fetching/streaming in the ConversationListView, but I can revert if you're not a fan!

Relates to #13

Before After (Light) After (Dark)
Simulator Screen Shot - iPhone 14 - 2023-02-01 at 09 22 58 Simulator Screen Shot - iPhone 14 - 2023-02-01 at 11 44 06 Simulator Screen Shot - iPhone 14 - 2023-02-01 at 11 43 57

@elisealix22 elisealix22 requested review from nakajima and m-005 February 1, 2023 16:57
Copy link
Contributor

@nakajima nakajima left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a few questions/comments but overall looks good!

xmtp-inbox-ios/Views/AccountView.swift Outdated Show resolved Hide resolved
guard let url = URL(string: privacyUrl) else {
return
}
UIImpactFeedbackGenerator(style: .heavy).impactOccurred()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be nice to maybe extract a HapticButton component since we're doing this in a few places? Maybe something like:

struct HapticButton<Content: View>: View {
	var action: () -> Void
	@ViewBuilder var label: () -> Content

	var body: some View {
		Button(action: {
			UIImpactFeedbackGenerator(style: .heavy).impactOccurred()
			action()
		}) {
			label()
		}
	}
}

Lemme know if you think that's overkill.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added and migrated most views over to it! Brilliant idea.

EnsImageView(imageSize: 40.0, peerAddress: client.address)
})
.navigationBarItems(leading: EnsImageView(imageSize: 40.0, peerAddress: client.address)
.onTapGesture {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this be a button instead of an onTapGesture? I think it'll work better with VoiceOver in that case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ended up using the new HapticButton 😁

xmtp-inbox-ios/Views/MessageListView.swift Outdated Show resolved Hide resolved
xmtp-inbox-ios/Views/MessageListView.swift Outdated Show resolved Hide resolved
@elisealix22 elisealix22 merged commit 7fd8176 into main Feb 1, 2023
@elisealix22 elisealix22 deleted the ea/account-view branch February 1, 2023 18:38
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants